Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2/n Nested Text textAlignVertical (Android) - adding textAlignVertical prop to NestedText #35704

Closed
wants to merge 65 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Dec 22, 2022

Summary

Requires merging 1/n Nested Text textAlignVertical (Android) - Adding CustomStyleSpan API to align nested text vertically PR #35949.

2/n of a series of pull requests (Nested Text textAlignVertical):

  • adds textAlignVertical prop to nested text (JavaScript, CPP, Java)
  • adds logic in ReactTextViewManager#updateExtraData to update CustomStyleSpan maxLineHeight and maxFontSize. maxLineHeight and maxFontSize are used to align vertically nested text
  • updates the span maxLineHeight and maxFontSize to align the nested text
  • MetricAffectingSpan method updateDrawState changes the TextPaint baseline and aligns the text top or bottom

CSS - vertical-align with spans

The CSS vertical-align property vertically aligns text on a character level

Code example CSS vertical-align

#30375 (comment)
https://jsfiddle.net/2duoLyq4/14/

<p class="paragraph">
<span>Lorem ipsum</span><span class="super">sup</span><img src="https://plchldr.co/i/245x155?bg=EB6361" />
</p>
<p class="paragraph">
<span>Lorem ipsum</span><span class="top">top</span><img src="https://plchldr.co/i/245x155?bg=EB6361" />
</p>

Android - explanation of the corresponding Span Android APIs

MetricAffectingSpan allows to vertically align text (group of characters, not paragraphs divided by \n) by changing the TextPaint baseline of a span.

Metric Affecting spans. Spans can also affect text metrics, such as line height and size. All of these spans extend the MetricAffectingSpan class.

Text is represented as TextView on Android, Nested Text is represented as Spans.
https://developer.android.com/develop/ui/views/text-and-emoji/spans

https://developer.android.com/develop/ui/views/text-and-emoji/spans#span-types

  • How the span affects text: a span can affect either text appearance or text metrics.
  • Span scope: some spans can be applied to individual characters, while others must be applied to an entire paragraph

They consist of four types:

  • Paragraph Affecting spans (text separated by \n (new line)). A span can also affect text at a paragraph level, such as changing the alignment or the margin of the entire block of text. Spans that affect entire paragraphs implement ParagraphStyle.
  • Character Affecting spans. A span can affect text at a character level. For example, you can update character elements like background color, style, or size. Spans that affect individual characters extend the CharacterStyle class.
  • Appearance Affecting spans
  • Metric Affecting spans. Spans can also affect text metrics, such as line height and text size. All of these spans extend the MetricAffectingSpan class.

Android does not support CSS vertical-align on character-level

An example of similar functionality on Android is the AlignmentSpan.
A paragraph is a text separated by \n (new line). The AlignmentSpan changes the horizontal alignment of the paragraph.

https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/text/Layout.java;l=494-500;drc=caabd536e8fea82b888902f359b33e637267966c;bpv=1;bpt=1?q=ALIGN_OPPOSITE&ss=android%2Fplatform%2Fsuperproject

New batch of paragraph styles, collect into spans array. Compute the alignment, last alignment style wins. Reset tabStops, we'll rebuild if we encounter a line with tabs. We expect paragraph spans to be relatively infrequent, use spanEnd so that we can check less frequently. Since paragraph styles ought to apply to entire paragraphs, we can just collect the ones present at the start of the paragraph. If spanEnd is before the end of the paragraph, that's not our problem.

Details on how the Layout.java API draws text with the AlignmentSpan.
#30375 (comment).

  1. Saves the paragraph alignment (writeToParcelInternal) as ALIGN_CENTER, ALIGN_OPPOSITE or ALIGN_NORMAL.
  2. Text and spans are drawn with Layout #drawText. The method iterates over each Nested Text and adds styles like background color, font size, and other nested Text attributes.
  3. drawText iterates over each AlignmentSpan and applies their custom horizontal alignment (source)
  4. The TextLine API is used to measure, recycle, and draw the text.

fixes #30375 #30375 (comment)

Changelog

[ANDROID] [ADDED] - 2/n Nested Text textAlignVertical (Android) - adding textAlignVertical prop to NestedText

Test Plan

#35704 (comment)
#35704 (comment)
#35704 (comment)

Previous Tests

#35704 (comment)
#35704 (comment) #35704 (comment)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 22, 2022
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Dec 22, 2022

P type task
1 test Verify issue still reproduce on the main branch
2 feature Use SuperScriptSpan to vertically position the Nested Text (link)
3 feature Copy fabric cpp changes from PR Adding Android support for Accessibility TtsSpan API with accessibilityRole and accessibilityUnit props #35130 Task 1) Add verticalAlign to TextAttributesPropsTask 2) Add Span to TextLayoutManagerMapBufferTask 3) Copy all the changes from PR #35130
4 feature Replace superscript and subscript with top and bottom (stackoverflow-1, stackoverflow-2)

P type task
1 test Verify issue still reproduce on the main branch
2 feature Use SuperScriptSpan to vertically position the Nested Text (link)
3 feature "Copy fabric cpp changes from PR Adding Android support for Accessibility TtsSpan API with accessibilityRole and accessibilityUnit props #35130
Task 1) Add verticalAlign to TextAttributesProps
Task 2) Add Span to TextLayoutManagerMapBuffer
Task 3) Copy all the changes from PR #35130"
4 feature Replace superscript and subscript with top and bottom (stackoverflow-1, stackoverflow-2)

@analysis-bot
Copy link

analysis-bot commented Dec 22, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,455,696 -13,762
android hermes armeabi-v7a 7,779,111 -12,549
android hermes x86 8,933,062 -11,541
android hermes x86_64 8,789,160 -12,865
android jsc arm64-v8a 6,669,828 -2,433,481
android jsc armeabi-v7a 7,463,682 -836,680
android jsc x86 9,195,080 +41,715
android jsc x86_64 6,894,964 -2,517,606

Base commit: bf03e86
Branch: main

@analysis-bot
Copy link

analysis-bot commented Dec 22, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 4d8ba7b
Branch: main

@fabOnReact
Copy link
Contributor Author

P type task
3 feature Copy fabric cpp changes from PR Adding Android support for Accessibility TtsSpan API with accessibilityRole and accessibilityUnit props #35130Task 1) Add verticalAlign to TextAttributesPropsTask 2) Add Span to TextLayoutManagerMapBufferTask 3) Copy all the changes from PR #35130
3.1 task RNTester crashes at startup caused by wrong configurations. Logcat points to newarchdefaults. Solution: gradle clear after making cpp changes.
3.2 task Log the value of textVerticalAlign for a nested text in TextAttributes.java to verify the value is passed from JS.
3.3 task Fix wrong mapping textVerticalAlign to textAlignVertical

P type task
3 feature "Copy fabric cpp changes from PR Adding Android support for Accessibility TtsSpan API with accessibilityRole and accessibilityUnit props #35130
Task 1) Add verticalAlign to TextAttributesProps
Task 2) Add Span to TextLayoutManagerMapBuffer
Task 3) Copy all the changes from PR #35130"
3.1 task RNTester crashes at startup caused by wrong configurations. Logcat points to newarchdefaults. Solution: gradle clear after making cpp changes.
3.2 task Log the value of textVerticalAlign for a nested text in TextAttributes.java to verify the value is passed from JS.
3.3 task Fix wrong mapping textVerticalAlign to textAlignVertical

@pull-bot
Copy link

PR build artifact for 7d147de is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 7d147de is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

…figs in TtsSpan, because the value mVerticalAlign is set in TextAttributesProps.java, but not in MapBuffer.

facebook#35704 (comment)
@fabOnReact

This comment was marked as duplicate.

@fabOnReact
Copy link
Contributor Author

P type task
1 Bug Verify configs behind mIsAccessibilityLink or check corresponding configs in TtsSpan, because the value mVerticalAlign is set in TextAttributesProps.java, but not in MapBuffer.
2 feature Modify Superscript implementation to align to the top (stackoverflow-1, stackoverflow-2)

P type task
1 Bug Verify configs behind mIsAccessibilityLink or check corresponding configs in TtsSpan, because the value mVerticalAlign is set in TextAttributesProps.java, but not in MapBuffer.
2 feature Modify Superscript implementation to align to the top (stackoverflow-1, stackoverflow-2)

@pull-bot
Copy link

PR build artifact for fdeb37c is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for fdeb37c is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@fabOnReact
Copy link
Contributor Author

P type task
1 bug Nested Text does not take height of parent and is not aligned as the parent
1.1 task Create flexbox example with Nested Text
1.2 task Retrieve element height with Text onLayout callback
1.3 task Set hardcorded height value in ReactTopAlignSpan constructor
1.4 task Use hardcorded height to position nested Text top/center/bottom
1.5 task Read about top, ascent, descent, bottom and leading (stackoverflow)
1.6 task Shift the baseline top/center/bottom using the hardcoded height (+100, +200 and -200) to position correctly the Text top/center/bottom. Take in consideration parent text alignment.
2 feature Implement Superscript and publish solution
2.1 task Implement previous superscript solution and stash the other changes
2.2 task adjustement using font height to adapt superscript to top

P type task
1 bug Nested Text does not take height of parent and is not aligned as the parent
1.1 task Create flexbox example with Nested Text
1.2 task Retrieve element height with Text onLayout callback
1.3 task Set hardcorded height value in ReactTopAlignSpan constructor
1.4 task Use hardcorded height to position nested Text top/center/bottom
1.5 task Read about top, ascent, descent, bottom and leading (stackoverflow)
1.6 task Shift the baseline top/center/bottom using the hardcoded height (+100, +200 and -200) to position correctly the Text top/center/bottom. Take in consideration parent text alignment.
2 feature Implement Superscript and publish solution
2.1 task Implement previous superscript solution and stash the other changes
2.2 task adjustement using font height to adapt superscript to top

@fabOnReact
Copy link
Contributor Author

Android - Basic implementation of Top and Bottom in nested text

does not support for height or lineHeight

@pull-bot
Copy link

PR build artifact for 5565bc1 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 5565bc1 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@fabOnReact
Copy link
Contributor Author

P type task
1 feature Update TopAlignSpan baseline position from ReactTextView#onDraw with the updated TextView height
1.1 task Retrieve in ReactTextView onDraw the text spans of class ReactTopAlignSpan with getSpans
1.2 task Loop over the spans and trigger method span.updateBaseline(height)
1.3 task Trigger updateBaseline and log the value of height from ReactTopAlignSpan
1.4 task Verify that the value is 550 or similar and correctly update the Span baseline
1.5 task ReactTopAlignSpan method updateBaselines updates the baseline based on the height received from ReactTextView getHeight
1.6 task Review (stackoverflow-1, stackoverflow-2)
1.7 task Try to draw the text manually with TextView onDraw to the correct position (slides)

P type task
1 feature Update TopAlignSpan baseline position from ReactTextView#onDraw with the updated TextView height
1.1 task Retrieve in ReactTextView onDraw the text spans of class ReactTopAlignSpan with getSpans
1.2 task Loop over the spans and trigger method span.updateBaseline(height)
1.3 task Trigger updateBaseline and log the value of height from ReactTopAlignSpan
1.4 task Verify that the value is 550 or similar and correctly update the Span baseline
1.5 task ReactTopAlignSpan method updateBaselines updates the baseline based on the height received from ReactTextView getHeight
1.6 task Review (stackoverflow-1, stackoverflow-2)
1.7 task Try to draw the text manually with TextView onDraw to the correct position (slides)

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Dec 28, 2022

Nested Text correctly aligns to the top of the View with flex style

<View style={{height: 250}}>
  <Text
    style={{
      flex: 1,
      textAlignVertical: 'bottom',
      backgroundColor: 'yellow',
    }}>
    A parent text with{' '}
    <Text
      style={{
        textAlignVertical: 'bottom-child',
        backgroundColor: 'red',
      }}>
      Bottom
    </Text>
    <Text
      style={{
        textAlignVertical: 'top-child',
        backgroundColor: 'green',
      }}>
      Top
    </Text>
  </Text>
  <Text
    style={{
      verticalAlign: 'bottom',
      backgroundColor: 'red',
    }}>
    Bottom
  </Text>
</View>

@pull-bot
Copy link

PR build artifact for 9de465b is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 9de465b is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@fabOnReact
Copy link
Contributor Author

P type task
2 bug Add logic to detect parent textAlignVertical and correctly shift baseline
2.1 task Test scenario with hardcoded values
2.2 task Add gravity to ReactTopAlignSpan constructor (the value retrieved with TextView#getGravity).
2.3 task Add parameter textAlignVertical to ReactTopAlignSpan constructor.
2.4 task Invalidate the ReactTopAlignSpan to trigger updateDrawState
2.5 task Change baseline based on the textAlignVertical, mParentGravity and mHeight.
2.6 task Rename ReactTopAlignSpan to ReactAlignSpan

@fabOnReact fabOnReact changed the title 🚧 [Android] add support for superscript to nested Text 🚧 [Android] add support for superscript to nested Text Dec 28, 2022
@fabOnReact fabOnReact changed the title [Android] add support for superscript to nested Text [Android] Adding Nested Text support for textAlignVertical Dec 28, 2022
@fabOnReact fabOnReact marked this pull request as ready for review February 21, 2023 08:25
@fabOnReact fabOnReact marked this pull request as draft February 21, 2023 08:25
}
}
for (CustomStyleSpan span : customStyleSpans) {
span.updateSpan(highestLineHeight, highestFontSize);
Copy link
Contributor Author

@fabOnReact fabOnReact Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.android.com/develop/ui/views/text-and-emoji/spans#best-practices

Calling view.setText(update) before updating the span. Result: text correctly aligns to the top or the bottom, the parent text vertical align does not change.
https://www.icloud.com/iclouddrive/0e3YRsM1KKEs0SXo0nvkEy9OQ#call_set_text_before_mutation

Calling view.setText(update) after updating the span. Result: text does not align correctly, The regression is caused by the change in the parent text vertical align.
https://www.icloud.com/iclouddrive/087kw0jNMTAFnT5pJ-mPHXzaQ#call_set_text_after_mutation

The logic is required to align spans correctly. The functionality works without issues.

@fabOnReact
Copy link
Contributor Author

P type task
1 review Update PR Summary with links and info
2 review Call setText with new new span. Don’t update existing span (android docs, code-review). Read the android docs if not clear (summary in PR)
2.1 task Review ReactTextViewManager #updateExtraData logic that retrieves the lineHeight (link)
2.2 task Read more about Binder Inter Process Communication (elinux explanation, google search)
2.3 task Further investigate how AlignmentSpan #writeToParcelInternal
2.4 task Understand if Layout ALIGN_CENTER is used to align nested Text (Android Spans)
2.5 task Compare Layout #getLineRight and other methods with David Vacca implementation of inline images attachments.
2.6 task Further read 2021 investigation around AOSP implementation of AlignmentSpans (comment on GitHub issue)
2.7 task Layout #drawText draws the ParagraphSpans with different styles using TextLine #recycle is used to update the text line (textpaint..etc..)
2.8 task Read again best practices on using spans (link) and avoid mutation

P type task
1 review Update PR Summary with links and info
2 review Call setText with new new span. Don’t update existing span (android docs, code-review). Read the android docs if not clear (summary in PR)
2.1 task Review ReactTextViewManager #updateExtraData logic that retrieves the lineHeight (link)
2.2 task Read more about Binder Inter Process Communication (elinux explanation, google search)
2.3 task "Further investigate how AlignmentSpan #writeToParcelInternal, uses Parcel to change the span text alignment (left, center or right).
2.4 task Understand if Layout ALIGN_CENTER is used to align nested Text (Android Spans)
2.5 task Compare Layout #getLineRight and other methods with David Vacca implementation of inline images attachments.
2.6 task Further read 2021 investigation around AOSP implementation of AlignmentSpans (comment on GitHub issue)
2.7 task Layout #drawText draws the ParagraphSpans with different styles using TextLine #recycle is used to update the text line (textpaint..etc..)
2.8 task Read again best practices on using spans (link) and avoid mutation

If you need to change only an internal attribute of a mutable span, such as the bullet color in a custom bullet span, you can avoid the overhead from calling setText() multiple times by keeping a reference to the span as it's created. When you need to modify the span, you can modify the reference and then call either invalidate() or requestLayout() on the TextView, depending on the type of attribute that you changed.
@fabOnReact
Copy link
Contributor Author

P type task
1 bug Test again regression caused by calling setText after mutating span (GitHub comment). The parent text shifts down when changing child baseline.
1.1 task Set parent text gravity top and verify change in behaviour
1.2 task Comment logic in ReactTextView#setText and verify change in behaviour
2 bug Parent text ancestor changes the baseline, when child has textAlignVertical TOP (video).parentTextPaint.baseline += (highestLineHeight / 2) / 2;As a consequence of the change in baseline of the parent Text compoment, the child does not align to the top of the TextView.childTextPaint.baseline -= (highestLineHeight / 2) / 2;
3.1 hypothesis The issue is caused by parent Gravity.TOP. The parent text baseline changes when one of the child has textAlignVertical top or center.
3.2 task Debug CustomStyleSpan logic that changes the TextPaint #baseline. Log values of mCurrentText, mHighestLineHeight, textPaint.baseline
3.3 solution Reproduces without using Gravity.TOP. For example, using textAlignVertical: bottom, the child component does not correctly align (link). The child component correctly aligns top and bottom, but does not align correctly to the center.  Technically we don’t change anything with center, so this could be caused by missing update.
4 hypothesis The TextView has an invisible margin. The TextView height does not correspond to the yellow background. The Text can not go over the invisible margin.
4.1 task Use a lower value of highestLineHeight and verify if the parent component shifts (video).
5 hypothesis The shift of position is triggered by changing the length of the text. Changing the text from top to bottom, causes a change in the total length of the text and the position.
5.1 task Avoid changing the text of the string when you change the variable textAlignVertical (video).
6 hypothesis It is caused by buildSpannableFromFragment (setText)
6.1 task Remove logic from buildSpannableFromFragment.
6.2 solution It is not caused by logic in buildSpannableFromFragment.
7 hypothesis The issue seems to be caused by the first render. When the text is first rendered, the text is aligned to the top of the View. Changing the child baseline to -50 (shift top), will make it reach the top of the view and cause the baseline of the parent to change of +25. When the text is actually rendered, the parent text is positioned with gravity.CENTER, but the original shift of +25 on the baseline persists.
7.1 task Record a video, showing how changing the baseline causes child to reach the top and shift parent. The issue caused by the first render in React, and shifting the baseline when parent is not correctly positioned with Gravity.Center (video)
7.2 task Call setText before updating the span with span.updateSpan. The functionality should work without issues. Check the diff and revert changes until functionality works.
7.3 task Review bug fix for Text gravity commit
8 review Read implementation of setText and find logic that can fix problem with alignment

P type task
1 bug Test again regression caused by calling setText after mutating span (GitHub comment). The parent text shifts down when changing child baseline.
1.1 task Set parent text gravity top and verify change in behaviour
1.2 task Comment logic in ReactTextView#setText and verify change in behaviour
2 bug "Parent text ancestor changes the baseline, when child has textAlignVertical TOP (video).
parentTextPaint.baseline += (highestLineHeight / 2) / 2;
As a consequence of the change in baseline of the parent Text compoment, the child does not align to the top of the TextView.
childTextPaint.baseline -= (highestLineHeight / 2) / 2; "
3.1 hypothesis "The issue is caused by parent Gravity.TOP.
The parent text baseline changes when one of the child has textAlignVertical top or center. "
3.2 task "Debug CustomStyleSpan logic that changes the TextPaint #baseline.
Log values of mCurrentText, mHighestLineHeight, textPaint.baseline"
3.3 solution Reproduces without using Gravity.TOP. For example, using textAlignVertical: bottom, the child component does not correctly align (link). The child component correctly aligns top and bottom, but does not align correctly to the center. Technically we don’t change anything with center, so this could be caused by missing update.
4 hypothesis "The TextView has an invisible margin.
The TextView height does not correspond to the yellow background. The Text can not go over the invisible margin."
4.1 task Use a lower value of highestLineHeight and verify if the parent component shifts (video).
5 hypothesis "The shift of position is triggered by changing the length of the text.
Changing the text from top to bottom, causes a change in the total length of the text and the position."
5.1 task Avoid changing the text of the string when you change the variable textAlignVertical (video).
6 hypothesis It is caused by buildSpannableFromFragment (setText)
6.1 task Remove logic from buildSpannableFromFragment.
6.2 solution It is not caused by logic in buildSpannableFromFragment.
7 hypothesis "The issue seems to be caused by the first render.
When the text is first rendered, the text is aligned to the top of the View. Changing the child baseline to -50 (shift top), will make it reach the top of the view and cause the baseline of the parent to change of +25.
When the text is actually rendered, the parent text is positioned with gravity.CENTER, but the original shift of +25 on the baseline persists."
7.1 task Record a video, showing how changing the baseline causes child to reach the top and shift parent. The issue caused by the first render in React, and shifting the baseline when parent is not correctly positioned with Gravity.Center (video)
7.2 task "Call setText before updating the span with span.updateSpan.
The functionality should work without issues. Check the diff and revert changes until functionality works."
7.3 task Review bug fix for Text gravity commit
8 review Read implementation of setText and find logic that can fix problem with alignment

@fabOnReact fabOnReact marked this pull request as ready for review February 24, 2023 17:29
@fabOnReact fabOnReact force-pushed the nested-text-superscript branch from b42f981 to dbeecaa Compare February 24, 2023 18:06
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 24, 2023
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 24, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jan 31, 2024
@fabOnReact fabOnReact deleted the nested-text-superscript branch January 31, 2024 22:37
@fabOnReact
Copy link
Contributor Author

I deleted the branch for mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] textAlignVertical doesn't work on nested text
4 participants